Conversation
|
✅ Preview binaries are ready! To test with modules: |
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Nice work so far! I left a few comments and suggestions. Mainly about moving some concerns from httpapi to cmd/server. I think it would be helpful to have some tests based on real agent session restoration output to evaluate the screentracker changes.
| AllowedHosts: viper.GetStringSlice(FlagAllowedHosts), | ||
| AllowedOrigins: viper.GetStringSlice(FlagAllowedOrigins), | ||
| InitialPrompt: initialPrompt, | ||
| StatePersistenceCfg: httpapi.StatePersistenceCfg{ |
There was a problem hiding this comment.
Suggestion: Naming Cfg -> Config (no previous precedent for Cfg in this repo).
| } | ||
| } | ||
|
|
||
| pidFile := viper.GetString(PidFile) |
There was a problem hiding this comment.
Suggestion: I think it'd make sense to move pid file handling here rather than httpapi as it becomes a bit disconnected and here we can write it early.
| // Handle SIGUSR1 for save without exit | ||
| saveOnlyCh := make(chan os.Signal, 1) | ||
| signal.Notify(saveOnlyCh, syscall.SIGUSR1) | ||
| go func() { |
There was a problem hiding this comment.
| go func() { | |
| go func() { | |
| defer signal.Stop(saveOnlyCh) |
Suggestion: Good practice to unregister on teardown.
| currentStatus = st.ConversationStatusChanging | ||
| s.logger.Info("Initial prompt sent successfully") | ||
| if !s.stateLoadComplete && s.statePersistenceCfg.LoadState { | ||
| _, _ = s.conversation.LoadState(s.statePersistenceCfg.StateFile) |
There was a problem hiding this comment.
Not objecting, just curious. Why do we wait for stability to load the state?
| s.cleanupPIDFile() | ||
|
|
||
| // Now close the process | ||
| if err := process.Close(s.logger, 5*time.Second); err != nil { |
There was a problem hiding this comment.
It feels a bit strange to control the process from this "far in". To me it would make sense to invert some of this control, i.e. change how things are wired up in cmd/server.
If we close here, won't we likely be logging an error in cmd/server due to the process being killed or some such? If so, we could improve the UX.
| s.logger.Info("Saved conversation state", "source", source, "stateFile", s.statePersistenceCfg.StateFile) | ||
| } | ||
| } else { | ||
| s.logger.Warn("Save requested but state saving is not configured", "source", source) |
There was a problem hiding this comment.
Won't this print save requested for regular stop signals like SIGTERM? I think this log is only applicable for USR1.
| func (s *Server) HandleSignals(ctx context.Context, process *termexec.Process) { | ||
| // Handle shutdown signals (SIGTERM, SIGINT only on Windows) | ||
| shutdownCh := make(chan os.Signal, 1) | ||
| signal.Notify(shutdownCh, os.Interrupt, syscall.SIGTERM) |
There was a problem hiding this comment.
Does this compile on Windows? IIRC we can only support os.Interrupt there.
3 participants
Closes: coder/internal#1256
MergeAfter: #172